fix(core): load the native addon on every Node major - #92
Open
not-matthias wants to merge 4 commits into
Open
Conversation
The addon shipped ABI-pinned prebuilds for Node 22 and 24 only, so on any other major node-gyp-build found no candidate, the binding silently fell back to the no-op stub and every benchmark failed with "Native core module is not bound". Only one V8 entry point stood in the way of a single Node-API prebuild: v8::String::Utf8Value gained a defaulted argument in Node 24 and WriteUtf8 gave way to WriteUtf8V2 in Node 26, so no string conversion symbol resolves on all three. Every other V8 symbol the perf-map handler needs is stable across 22, 24 and 26. Route the conversion through Node-API and ship one node.napi.node again. prebuildify 6 defaults --name to the package name and no longer appends the napi tag, which node-gyp-build 4.6 rejects, hence the explicit --name.
The require error was swallowed into a debug log, so a failed binding gave no clue whether the prebuild was missing, incompatible or broken. Carry the error to setupCore and include the runtime it was rejected for.
The existing jobs compile the addon with the same Node they then run it under, so an incompatible prebuild cannot show up there. Build one prebuild set and load it from each major a consumer may run, calling setupCore so that lazily bound V8 symbols are resolved rather than only opening the file.
not-matthias
force-pushed
the
fix/napi-prebuilds
branch
from
August 27, 2026 17:14
cfbab0d to
d344cb8
Compare
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | switch 2 |
432 B | 656 B | -34.15% |
| ⚡ | Simulation | recursive fibo 10 |
1,371.7 µs | 298.5 µs | ×4.6 |
| ⚡ | WallTime | test_iterative_fibo_10 |
120 ns | 96 ns | +25% |
| ⚡ | WallTime | switch 1 |
84 ns | 72 ns | +16.67% |
| ⚡ | WallTime | test sync baz 10 |
108 ns | 96 ns | +12.5% |
| ⚡ | WallTime | short body |
2.1 µs | 1.9 µs | +10.06% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/napi-prebuilds (9f111ed) with main (9338d9a)
not-matthias
marked this pull request as ready for review
August 28, 2026 08:28
Greptile SummaryThis PR replaces ABI-specific native-addon builds with one Node-API-tagged prebuild and adds Node 22/24 compatibility validation and richer binding diagnostics.
Confidence Score: 4/5The native string conversion must be fixed before merging because routine V8 code events can cause an out-of-bounds native write. The new Node-API conversion allocates storage for the UTF-8 payload but tells Node-API that an additional terminator byte is writable, exposing benchmark processes to memory corruption while perf-map names are generated. Files Needing Attention: packages/core/src/native_core/linux_perf/utils.h
|
| Filename | Overview |
|---|---|
| packages/core/src/native_core/linux_perf/utils.h | Replaces unstable V8 conversion symbols with Node-API, but supplies a destination size one byte larger than the allocated string storage. |
| packages/core/src/native_core/linux_perf/linux_perf_listener.cc | Threads the Node-API environment into function and script-name conversion; its live code-event paths expose the conversion overflow. |
| packages/core/package.json | Changes native packaging to a single explicitly named N-API prebuild compiled against Node 22 headers. |
| .github/workflows/ci.yml | Adds a focused Linux job that builds one native artifact and exercises it under both supported Node majors. |
| scripts/assert-native-binding.cjs | Exercises lazy native symbol binding and verifies that Linux perf produces map entries. |
| packages/core/src/index.ts | Improves native-binding failure diagnostics with the underlying error and runtime ABI/platform details. |
Sequence Diagram
sequenceDiagram
participant CI as Native ABI CI
participant Build as prebuildify
participant Loader as node-gyp-build
participant Core as setupCore
participant Perf as LinuxPerf
participant NAPI as Node-API
CI->>Build: Build node.napi.node against Node 22
loop Node 22 and Node 24
CI->>Loader: Load shared prebuild
Loader->>Core: Bind native_core
Core->>Perf: Start code-event handler
Perf->>NAPI: Convert V8 names to UTF-8
Core->>Perf: Stop handler
end
Prompt To Fix All With AI
### Issue 1
packages/core/src/native_core/linux_perf/utils.h:25-29
**UTF-8 conversion overruns buffer**
When a V8 code event has a nonempty function or script name, this allocates `length` writable characters but tells `napi_get_value_string_utf8` that `length + 1` bytes are available, causing the trailing NUL to be written beyond the string's guaranteed storage and potentially crashing or corrupting the benchmark process.
```suggestion
std::string result(length + 1, '\0');
if (napi_get_value_string_utf8(env, value, result.data(), result.size(),
&length) != napi_ok) {
return std::string();
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "ci: check the prebuilt addon binds on ev..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ship a single Node-API prebuild for the native core, so one binary covers both supported Node majors instead of one binary per ABI.
Since
c5c63cathe addon has shipped ABI-pinned prebuilds (node.abi127.node,node.abi137.node).node-gyp-buildmatches those on an exactprocess.versions.modules, so on any other major it finds no candidate,native_coresilently falls back to the no-op stub, and every benchmark dies withNative core module is not bound, CodSpeed integration will not work properly. That contradicts whatnodeVersion.tsalready tells users, which is that other versions are experimental rather than fatal.Only one V8 entry point stood in the way of a single prebuild.
v8::String::Utf8Value's constructor gained a defaultedWriteOptionsargument in Node 24, andWriteUtf8gave way toWriteUtf8V2in Node 26, so no single string-conversion symbol resolves across majors:String::Utf8Value::Utf8Value(Isolate*, Local<Value>)String::Utf8Length/String::WriteUtf8String::WriteUtf8V2Note the middle row: reverting to the pre-
c591e88formulation would satisfy 22 and 24 today, but it is the one that V8 has already removed, so it re-arms the same failure for the next bump. The other fourteen V8 symbols the perf-map handler needs (CodeEventHandlerctor/dtor/Enable/Disable, the eightCodeEventgetters,Isolate::GetCurrent) are present in every binary checked. So the conversion goes through Node-API, which is versioned and does not move.Two packaging details worth attention during review:
--nameto the package name and dropped thenapifilename tag, contradicting its own README. A bare--napiemits@codspeed+core.nodeornode.node, both of whichnode-gyp-build@4.6.0rejects (if (tags.abi !== abi && !tags.napi) return false). Hence the explicit--name node.napi.22.0.0so we compile against the oldest supported headers rather than whatevernode-abicurrently believes is newest, which today is 26.setupCorealso now reports the underlyingrequirefailure plus the Node version, ABI and platform. Previously it went tologDebug, which is why the original report gave nothing to work from.Why CI did not catch this
The
node-versionsjob runspnpm turbo run build, which includesbuild-native-addon, under the same Node it then benchmarks with. The addon is always compiled for the running ABI, so an incompatible prebuild is structurally unobservable there. The newnative-abijob builds one prebuild set and loads it from Node 22 and 24, callingsetupCorebecause the symbols bind lazily: a broken addon passes a barerequireand only dies onceLinuxPerf::Startruns.Verified locally against both failure modes on a from-scratch build:
--napiwith the oldUtf8Valuecall: Node 24 exits 127 onundefined symbol: _ZN2v86String9Utf8ValueC1EPNS_7IsolateENS_5LocalINS_5ValueEEE, while 22 passes.Scope
Both designs satisfy the 22 and 24 requirement, so the argument for this one is what happens outside it: users on another major degrade with the existing experimental-version warning instead of hitting an unreadable stub failure, and the package ships three prebuilt binaries instead of six. The tradeoff is that the two supported majors now share one binary and therefore depend on those fourteen V8 symbols staying put, rather than each being compiled against its own headers. That is the same bet the v5 line ran successfully for three years, and the new job fails loudly if it ever stops holding.
This does not remove the V8 C++ dependency. Retiring it entirely means dropping
LinuxPerffor--perf-basic-prof, which the walltime path already uses and which writes the same/tmp/perf-<pid>.map.